Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

32-bit support #232

Merged
merged 4 commits into from
Jan 9, 2024
Merged

32-bit support #232

merged 4 commits into from
Jan 9, 2024

Conversation

lfittl
Copy link
Member

@lfittl lfittl commented Dec 30, 2023

Fixes #231

This can be useful when building for WASM targets, as well as for legacy
systems that run on 32-bit.

The relevant code is enabled at compile time by checking the pointer
size (__SIZEOF_POINTER__ == 4).
This mainly helps with resolving compiler warnings, and should not have
any effective difference, since the relevant struct (RTEPermissionInfo)
is not actually used in raw parsing.
This was referenced Dec 30, 2023
@@ -1272,7 +1272,7 @@ message RTEPermissionInfo
{
uint32 relid = 1 [json_name="relid"];
bool inh = 2 [json_name="inh"];
int64 required_perms = 3 [json_name="requiredPerms"];
uint64 required_perms = 3 [json_name="requiredPerms"];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in the commit message, this is okay to do only because this struct isn't actually used in raw parse trees. Otherwise this would be an API break (which may be okay in the context of how we use these protobufs, but at least we should talk about doing this outside of major releases).

@lelit
Copy link
Contributor

lelit commented Dec 31, 2023

FYI, I tried out this PR and it fixes the problem, all CI tests are green again!
Thank you!

We've previously managed these in the Makefile inline, but that is
very hard to read, and not necessary, since we can simply concatenate
the override file to pg_config.h during the source extraction process.
@lfittl lfittl requested a review from a team January 2, 2024 18:31
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit tricky to verify this with go-pgquery since I use the C sources from pg_query_go instead of these ones to make sure they align, but I think this should fix my issue well. Thanks a lot!

@lfittl lfittl requested a review from msepga January 8, 2024 21:54
@lfittl lfittl merged commit 980f48b into 16-latest Jan 9, 2024
16 checks passed
@anuraaga
Copy link
Contributor

Just to close the loop, after updating I could remove my 32-bit support patch. Thanks!

https://github.com/wasilibs/go-pgquery/pull/22/files#diff-4d79e217dbbb7c7f611bfd2e2a6111d14e17bff3f3dbafac24d6314e5eeafc03L58

@keiko713 keiko713 deleted the 32-bit-support branch January 11, 2024 08:29
@keiko713 keiko713 restored the 32-bit-support branch January 11, 2024 08:30
@lfittl lfittl deleted the 32-bit-support branch January 11, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

32-bit support
4 participants